-
Notifications
You must be signed in to change notification settings - Fork 418
Introduce FundingTransactionReadyForSignatures
event
#3889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
let witnesses: Vec<_> = transaction | ||
.input | ||
.into_iter() | ||
.filter_map(|input| if input.witness.is_empty() { None } else { Some(input.witness) }) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a strong opinion here, but seems we can avoid this by passing the Transaction
through and only collecting witnesses when we are ready to construct TxSignatures
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, yeah will look at changing this after fixups.
165d59d
to
8ca6d79
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3889 +/- ##
==========================================
- Coverage 88.88% 88.82% -0.07%
==========================================
Files 165 165
Lines 118886 118971 +85
Branches 118886 118971 +85
==========================================
Hits 105676 105676
- Misses 10892 10976 +84
- Partials 2318 2319 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
890633d
to
a1de384
Compare
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
7df5779
to
c8f981c
Compare
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there was a problem rebasing, but some comments that had been marked resolved weren't fixed.
Yeah, they got lost on a rebase and somehow lost the commit. Rebased to get the one CI fix in. Fixing. |
c15f426
to
ff1489d
Compare
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
ff1489d
to
0a586e6
Compare
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
fn verify_interactive_tx_signatures(&mut self, _witnesses: &Vec<Witness>) { | ||
if let Some(ref mut _signing_session) = self.interactive_tx_signing_session { | ||
// Check that sighash_all was used: | ||
// TODO(dual_funding): Check sig for sighash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this isn't done yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah we're actually unable to do this reliably, especially if inputs were spend P2WSH. The public comments warn the client to ensure it is SIGHASH_ALL
(or SIGHASH_DEFAULT/SIGHASH_ALL) if spending a P2TR UTXO.
These TODOs need to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to use ScriptBuf::instructions
to identify all pushes that match a signature length, parse those that match, and make sure they come with SIGHASH_ALL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried we assume something is a sig just based on length when it's not. I mean it's probably a sig, but we'd then actually need to check if there is a corresponding checksig in the script.
If it wasn't a sig then we'd always treat that as invalid if it happens to have the same length and something that looks like a sighash_all byte (or the case no explicit sighash as in p2tr sighash_default).
It's probably unlikely but I haven't considered how "probably".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ECDSA sigs we can be a bit more sure since they have to parse as DER-encoded. P2WSH and P2TR-script-spend always reveal the witness script so we can certainly check for corresponding OP_CHECKSIG
s there. P2WPKH and P2TR-key-spend have a specific index within the witness that corresponds to the signature.
If we don't check this for holder witnesses, then we're susceptible to malleability attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it seems doable with what the bitcoin
crate provides. Been working on it already. Was looking for prior work but can't spot anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have this so far in draft, but obviously the P2TR script path spend needs to check for corresponding checksig(verify)/checksigadds as anything 64 bytes (65 if not using SIGHASH_DEFAULT) can be deserialised as a possible signature. In P2WSH we don't need to check that.
Needs some clean up for error handling and dedup / split up for easier testing / comments. Just commenting here fyi.
A few other things:
- We checked that all prev outputs are "standard" during tx construction, so if a witness is not one of the recognised witnesses shall we just ignore and not error? (considering it'll fail anyway). I'd say we should only care about checking for sighash types for witnesses corresponding to "standard" scriptpubkeys.
- The function name should probably include "sighash_type" to make it clear that's all we check.
fn verify_interactive_tx_signatures
fn verify_interactive_tx_signatures(
&mut self, witnesses: &Vec<Witness>,
) -> Result<(), APIError> {
if let Some(ref mut signing_session) = self.interactive_tx_signing_session {
// We get the (already sorted) inputs' prevout scriptpubkeys corresponding to the provided
// witnesses.
let script_pubkeys = signing_session
.unsigned_tx()
.inputs()
.filter(|input| matches!(input, InteractiveTxInput::Local(_)))
.map(|input| &input.prev_output().script_pubkey);
for (idx, (script_pubkey, witness)) in script_pubkeys.zip(witnesses).enumerate() {
// Check ECDSA signatures (P2WPKH, P2WSH)
if script_pubkey.is_p2wpkh() {
if witness
.iter()
.next()
.and_then(|sig_bytes| {
bitcoin::ecdsa::Signature::from_slice(sig_bytes)
.ok()
.map(|sig| matches!(sig.sighash_type, EcdsaSighashType::All))
})
.unwrap_or(false)
{
continue;
} else {
return Err(APIError::APIMisuseError {
err:
format!("The signature in the P2WPKH witness at index {idx} does not use SIGHASH_ALL")
});
}
}
if script_pubkey.is_p2wsh() {
for instruction in
witness.witness_script().iter().flat_map(|script| script.instructions())
{
match instruction {
Ok(bitcoin::script::Instruction::PushBytes(push_bytes)) => {
if let Some(sig) =
bitcoin::ecdsa::Signature::from_slice(push_bytes.as_bytes())
.ok()
{
if !matches!(sig.sighash_type, EcdsaSighashType::All) {
return Err(APIError::APIMisuseError {
err:
format!("A signature in the P2WSH witness at index {idx} does not use SIGHASH_ALL")
});
}
}
},
_ => continue,
}
}
}
if script_pubkey.is_p2tr() {
// If the witness contains a single element, it corresponds to a P2TR key path spend
if witness.len() == 1 {
if let Some(sig) = bitcoin::taproot::Signature::from_slice(&witness[0]).ok()
{
if !matches!(
sig.sighash_type,
TapSighashType::Default | TapSighashType::All
) {
return Err(APIError::APIMisuseError {
err:
format!("The signature for the key path spend in the P2TR witness at index {idx} does not use SIGHASH_ALL or SIGHASH_DEFAULT")
});
}
}
}
// Else we assume a script path spend
for instruction in witness
.taproot_leaf_script()
.iter()
.flat_map(|leaf_script| leaf_script.script.instructions())
{
match instruction {
Ok(bitcoin::script::Instruction::PushBytes(push_bytes)) => {
if let Some(sig) =
bitcoin::taproot::Signature::from_slice(push_bytes.as_bytes())
.ok()
{
if !matches!(
sig.sighash_type,
TapSighashType::All | TapSighashType::Default
) {
return Err(APIError::APIMisuseError {
err:
format!("A signature for the script path spend in the P2TR witness at index {idx} does not use SIGHASH_ALL or SIGHASH_DEFAULT")
});
}
}
},
_ => continue,
}
}
}
}
}
Ok(())
}
The `FundingTransactionReadyForSignatures` event requests witnesses from the client for their contributed inputs to an interactively constructed transaction. The client calls `ChannelManager::funding_transaction_signed` to provide the witnesses to LDK.
0a586e6
to
a9e1a3a
Compare
We directly get the holder `TxSignatures` when necessary.
a9e1a3a
to
83e78d6
Compare
Cherry-picked from #3735 as it is relevant to splicing and will unblock testing after #3736 lands.
The
FundingTransactionReadyForSignatures
event requests witnesses from the client for their contributed inputs to an interactively constructed transaction.The client calls
ChannelManager::funding_transaction_signed
to provide the witnesses to LDK.